Skip to content

fix(unparser): unparse a stacked aggregate as a derived table - #192

Open
claudespice wants to merge 1 commit into
spiceai:spiceai-54from
claudespice:fix/8475-stacked-aggregate-unparse
Open

fix(unparser): unparse a stacked aggregate as a derived table#192
claudespice wants to merge 1 commit into
spiceai:spiceai-54from
claudespice:fix/8475-stacked-aggregate-unparse

Conversation

@claudespice

Copy link
Copy Markdown

Summary (root cause)

A SELECT expresses a single grouping, but the LogicalPlan::Aggregate arm of
select_to_sql_recursively recursed straight into its input whenever the select list had
already been built (select.already_projected()), so a second aggregate stacked
underneath was skipped entirely
— its GROUP BY never reached the emitted SQL. The
Sort, Limit, Distinct and Projection arms all guard the same situation by emitting
a derived table; Aggregate did not.

single_distinct_to_groupby produces exactly that shape for count(DISTINCT c): an outer
count(alias1) over an inner Aggregate grouping by c AS alias1. Unparsing the
optimized plan for SELECT count(DISTINCT "UserID") FROM hits therefore emitted

SELECT count(alias1) AS c FROM hits          -- inner GROUP BY silently gone

Two consequences, both bad:

  1. alias1 does not exist on the base table, so a consumer that pushes the optimized plan
    down to a remote engine gets a binder error (Referenced column "alias1" not found in FROM clause). This is the failure reported downstream in DuckDB agg pushdown: Clickbench/aliasing issues spiceai#8475 against
    ClickBench q5 on DuckDB.
  2. Where a column of that name does exist, the statement binds and the DISTINCT is
    silently gone — count(alias1) counts every row.

Changes

  • SelectBuilder tracks whether an aggregate has been folded into the current SELECT
    (mark_aggregated() / already_aggregated()).
  • The Aggregate arm emits a derived_aggregate table for a second aggregate instead of
    skipping it, matching the existing derived_sort / derived_limit / derived_distinct /
    derived_projection handling.

After the fix:

SELECT count(alias1) AS c FROM (SELECT hits."UserID" AS alias1 FROM hits GROUP BY hits."UserID")

Why the existing roundtrip suites missed it

datafusion/core/tests/sql/unparser.rs runs the TPC-H and ClickBench queries through
df.logical_plan() — the plan as the SQL planner produces it, before
single_distinct_to_groupby runs. The rewrite only appears in the optimized plan, which
is what a federation/pushdown consumer unparses. The new test closes that gap.

Test plan

  • cargo test -p datafusion-sqlstacked_aggregate_is_unparsed_as_a_derived_table
    covers the ungrouped count(DISTINCT b) shape, the grouped
    a, count(DISTINCT b) ... GROUP BY a shape, and a lone aggregate (which must still fold
    into its SELECT rather than become a derived table).
    Result: 498 passed / 22 failed, versus 497 / 22 on a pristine checkout of this
    base — the same 22 pre-existing failures, byte-identical output, plus the new test.
  • cargo test -p datafusion --test core_integration sql::
    test_optimized_plan_roundtrip_count_distinct unparses the optimized plan for
    count(DISTINCT) with and without a GROUP BY, then executes both the original and the
    unparsed SQL against the ClickBench fixture and compares the rows.
    Result: 71 passed / 29 failed, versus 70 / 30 on the same base — the only delta
    is the new test flipping from failing to passing.
  • Neutering check: with plan.rs reverted, both new tests fail, and the core test fails
    with column 'alias1' not found and the emitted SQL
    SELECT count(alias1) AS c FROM hits_raw AS hits — i.e. it reproduces the reported bug.

Downstream issue: spiceai/spiceai#8475

A SELECT expresses a single grouping, but the `LogicalPlan::Aggregate` arm of
`select_to_sql_recursively` recursed straight into its input whenever the select
list was already built, so a second aggregate underneath was skipped and its
`GROUP BY` never reached the emitted SQL.

`single_distinct_to_groupby` produces exactly that shape for `count(DISTINCT c)`:
an outer `count(alias1)` over an inner `Aggregate` grouping by `c AS alias1`. With
the inner aggregate dropped, `SELECT count(DISTINCT "UserID") FROM hits` unparses
to `SELECT count(alias1) FROM hits` — `alias1` does not exist on the base table, so
a consumer pushing the optimized plan down to a remote engine gets a binder error,
and where a column of that name does exist it counts every row rather than the
distinct values.

Track on the `SelectBuilder` whether an aggregate has been folded into the current
SELECT, and emit a `derived_aggregate` table for the next one, as the `Sort`,
`Limit`, `Distinct` and `Projection` arms already do.

The existing TPC-H and ClickBench roundtrip suites unparse the plan as the SQL
planner produces it, before `single_distinct_to_groupby` runs, which is why they
never caught this; the new core test unparses the optimized plan and executes both
statements.
@claudespice

Copy link
Copy Markdown
Author

This PR has been open and green for 5.6 days with no reviewer ever requested and no review activity. For context, recent merges in this repo landed in 15 minutes to ~2.7 days (#181#189), so this is well outside the normal window.

I don't have write access on this fork, so I can't add a reviewer or assignee myself — flagging it here instead. Two siblings are in the same state: #190 (physical-optimizer) and #191 (unparser).

Nothing is blocking it technically: CI is green and it merges cleanly into spiceai-54. It just needs someone to take a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant